fix: add support for vault_secret_key - #421
Conversation
- When creating or updating credentials with HashiCorp vault secrets, require the vault_key when users specify the vault_secret_path.
Reviewer's GuideThis PR adds explicit support for a required HashiCorp Vault key when using vault-based credentials, wiring a new --vault-key CLI option through argument parsing, validation, payload construction, tests, and documentation for both cred add and cred edit flows. Sequence diagram for cred add/edit with required vault_keysequenceDiagram
actor User
participant qpc_cred_add as qpc_cred_add
participant qpc_cred_edit as qpc_cred_edit
participant validate_vault_args as validate_vault_args
participant build_credential_payload as build_credential_payload
User->>qpc_cred_add: qpc cred add --vault-secret-path path --vault-key key
qpc_cred_add->>validate_vault_args: validate_vault_args(args, cred_type)
validate_vault_args-->>qpc_cred_add: args validated
qpc_cred_add->>build_credential_payload: build_credential_payload(args, cred_type, add_none)
build_credential_payload-->>qpc_cred_add: payload with vault_secret_path and vault_key
User->>qpc_cred_edit: qpc cred edit --vault-secret-path path --vault-key key
qpc_cred_edit->>validate_vault_args: validate_vault_args(args, cred_type)
validate_vault_args-->>qpc_cred_edit: args validated
qpc_cred_edit->>build_credential_payload: build_credential_payload(args, cred_type, add_none)
build_credential_payload-->>qpc_cred_edit: payload with vault_secret_path and vault_key
alt [vault_secret_path set and vault_key missing]
qpc_cred_add->>validate_vault_args: validate_vault_args(args, cred_type)
validate_vault_args->>validate_vault_args: logger.error(CRED_VAULT_KEY_REQUIRED)
validate_vault_args->>validate_vault_args: sys.exit(1)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
+ Coverage 96.47% 96.49% +0.01%
==========================================
Files 138 138
Lines 8690 8737 +47
==========================================
+ Hits 8384 8431 +47
Misses 306 306 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
validate_vault_args, you enforce thatvault_keyis required whenvault_secret_pathis set, but there is no reciprocal check; consider adding a guard that errors if--vault-keyis provided without--vault-secret-path(similar to the existingvault_mount_pointvalidation) to prevent inconsistent CLI usage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validate_vault_args`, you enforce that `vault_key` is required when `vault_secret_path` is set, but there is no reciprocal check; consider adding a guard that errors if `--vault-key` is provided without `--vault-secret-path` (similar to the existing `vault_mount_point` validation) to prevent inconsistent CLI usage.
## Individual Comments
### Comment 1
<location path="qpc/cred/utils.py" line_range="175-178" />
<code_context>
"""
# Get vault options (use getattr for legacy test compatibility)
vault_secret_path = getattr(args, "vault_secret_path", None)
+ vault_key = getattr(args, "vault_key", None)
vault_mount_point = getattr(args, "vault_mount_point", None)
</code_context>
<issue_to_address>
**issue (bug_risk):** Validate that --vault-key is not provided without --vault-secret-path
`validate_vault_args` currently requires `vault_key` when `vault_secret_path` is set, but not the reverse: `--vault-key` can be passed alone, leaving `vault_secret_path` falsy and bypassing validation. This permits an invalid config that contradicts the documented usage.
Please add a check such as:
```python
if vault_key and not vault_secret_path:
logger.error(_(messages.CRED_VAULT_KEY_REQUIRES_PATH))
sys.exit(1)
```
(or reuse an existing message) so `--vault-key` is rejected unless `--vault-secret-path` is also provided.
</issue_to_address>
### Comment 2
<location path="qpc/tests/cred/test_vault_cred_edit.py" line_range="144-153" />
<code_context>
+ def test_edit_vault_missing_key(
</code_context>
<issue_to_address>
**suggestion (testing):** Add an explicit test for `--vault-key` provided without `--vault-secret-path` to document and lock in the expected behavior.
You’ve covered the `--vault-secret-path` without `--vault-key` case. Please also add tests (for both `add` and `edit`) for the inverse: `--vault-key` provided without `--vault-secret-path`. That will lock in whether this should error or be ignored, and ensure the current behavior is validated via `sys.argv` and the resulting outcome (error or no-op).
Suggested implementation:
```python
assert payload["vault_mount_point"] == "custom-mount"
def test_edit_vault_missing_secret_path_with_key(
self,
capsys,
requests_mock,
):
"""Test that vault key without vault secret path fails."""
url = get_server_location() + CREDENTIAL_URI
requests_mock.get(
url,
status_code=200,
json={
"name": "vault_cred",
"type": "vault",
"vault_secret_path": "secret/data/my-creds",
"vault_key": "existing-key",
"vault_mount_point": "secret",
},
)
# Simulate CLI invocation with --vault-key but no --vault-secret-path
sys.argv = [
"qpc",
"cred",
"edit",
"--name",
"vault_cred",
"--vault-key",
"my-new-key",
]
with pytest.raises(SystemExit) as exc:
main()
captured = capsys.readouterr()
# Lock in current behavior: non-zero exit when vault-key is provided
# without a matching vault-secret-path.
assert exc.value.code != 0
# Keep this assertion loose so it doesn't over-constrain messaging,
# but still documents that the error mentions vault arguments.
assert "vault" in captured.err.lower()
def test_edit_vault_missing_key(
self,
capsys,
requests_mock,
):
"""Test that vault secret path without vault key fails."""
url = get_server_location() + CREDENTIAL_URI
requests_mock.get(
url,
status_code=200,
json={
```
1. The new test `test_edit_vault_missing_secret_path_with_key` assumes:
- `sys` is imported (e.g., `import sys`) at the top of this file.
- `pytest` is imported (e.g., `import pytest`) at the top of this file.
- `main` is the CLI entrypoint already used in other tests in this module, and is imported accordingly.
- The existing tests in this file already follow the `sys.argv` + `main()` pattern; if they use a different runner helper, adjust the invocation in this new test to match (e.g., `runner.invoke(...)` or a project-specific helper).
2. To fully address your original review comment (“for both `add` and `edit`”), a symmetric test should be added to `qpc/tests/cred/test_vault_cred_add.py`, e.g.:
```python
def test_add_vault_missing_secret_path_with_key(capsys):
"""Test that vault key without vault secret path fails for add."""
sys.argv = [
"qpc",
"cred",
"add",
"--name",
"vault_cred",
"--type",
"vault",
"--vault-key",
"my-key",
]
with pytest.raises(SystemExit) as exc:
main()
captured = capsys.readouterr()
assert exc.value.code != 0
assert "vault" in captured.err.lower()
```
Place this new test alongside the existing `add`-side Vault tests and adjust the runner/imports to match the conventions in that file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Make sure we catch the case where vault_key is specified without having secified the vault_secret_path.
- Preferring to use vault_secret_key instead of vault_key as to not confuse with keys uses to authenticate to the vault itself.
|
Merging with the ci failure as this needs the API and camayoc PRs merged. |
Summary by Sourcery
Require a vault key when using HashiCorp Vault-backed credentials and propagate it through the CLI and payloads, with corresponding documentation and tests.
New Features:
Bug Fixes:
Documentation:
Tests: